Skip to content

gh-144490: Fix mimalloc debug build#144620

Merged
vstinner merged 2 commits intopython:mainfrom
yoney:cpp_mimalloc
Feb 10, 2026
Merged

gh-144490: Fix mimalloc debug build#144620
vstinner merged 2 commits intopython:mainfrom
yoney:cpp_mimalloc

Conversation

@yoney
Copy link
Contributor

@yoney yoney commented Feb 9, 2026 •
edited
Loading

Fix mimalloc C++ compilation error in debug builds.

The _mi_assert_fail declaration uses mi_decl_throw, which expands to __THROW (for __GNUC__ ). In C, __THROW is __attribute__((__nothrow__)) and works anywhere. In C++, __THROW is noexcept(true), which must come after the parameter list.

Before:

  // C:   __attribute__((__nothrow__)) void _mi_assert_fail(...);
  // C++: noexcept(true) void _mi_assert_fail(...);  <- Error, noexcept(...) must be after the params

After:

  // C:   void _mi_assert_fail(...) __attribute__((__nothrow__));
  // C++: void _mi_assert_fail(...) noexcept(true);

This fix also allows removing the Py_DEBUG guard in test_cppext/extension.cpp, so internal C API headers are now tested in debug builds too.

It’s possible this issue was introduced in #122587, though I haven’t verified it

Test:

$ ./configure --with-pydebug --disable-gil
$ make
$ ./python -m test test.test_cppext -u cpu=1

cc: @vstinner @colesbury @DinoV

@bedevere-app bedevere-app bot mentioned this pull request Feb 9, 2026
Closed
@yoney yoney marked this pull request as ready for review February 9, 2026 07:07
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Feb 9, 2026

Does this issue exists in mimalloc upstream as well?

@yoney
Copy link
Contributor Author

yoney commented Feb 9, 2026

Does this issue exists in mimalloc upstream as well?

It looks like upstream fixed this earlier. The change is semantically the same: they just moved mi_attr_noexcept to the end. However, the naming differs, our code uses mi_decl_throw, while upstream uses mi_attr_noexcept.

microsoft/mimalloc@715acc0

vstinner
vstinner approved these changes Feb 10, 2026
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I confirm that test_cppext.TestInteralCAPI pass on Linux with Python built in debug mode.

@vstinner
Copy link
Member

vstinner commented Feb 10, 2026

Note: test_cppext.TestInteralCAPI fails on Windows with Python built in release mode if I remove # if !defined(MS_WINDOWS) in Lib/test/test_cppext/extension.cpp. The change is correct: we should continue skipping the two pycore includes on Windows.

@vstinner vstinner merged commit 73fa6be into python:main Feb 10, 2026
51 checks passed
@vstinner vstinner added the needs backport to 3.14 bugs and security fixes label Feb 10, 2026
@miss-islington-app
Copy link

miss-islington-app bot commented Feb 10, 2026

Thanks @yoney for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

@miss-islington-app
Copy link

miss-islington-app bot commented Feb 10, 2026

Sorry, @yoney and @vstinner, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 73fa6be2fe6c4a17d91413e12ab6af8376767211 3.14

@vstinner vstinner removed the needs backport to 3.14 bugs and security fixes label Feb 10, 2026
@vstinner
Copy link
Member

vstinner commented Feb 10, 2026

Ah, the 3.14 branch is not affected, since the _mi_assert_fail() declaration doesn't use mi_decl_throw:

void _mi_assert_fail(const char* assertion, const char* fname, unsigned int line, const char* func );

@vstinner
Copy link
Member

vstinner commented Feb 10, 2026

Merged, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Assignees

@vstinner vstinner

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yoney @kumaraditya303 @vstinner